Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove native/internal LSTM model #600

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

mattw-nws
Copy link
Contributor

Builds on/requires #599

Additions

  • None

Removals

  • LSTM native/internal model, associated tests and data files.

Changes

  • Removals from some CMakeLists.txt files

Testing

  1. Passes all automated tests

Screenshots

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

  • Linux

@mattw-nws mattw-nws changed the title Remove LSTM Remove native/internal LSTM model Aug 2, 2023
@mattw-nws mattw-nws marked this pull request as ready for review August 3, 2023 16:13
@mattw-nws mattw-nws self-assigned this Aug 3, 2023
PhilMiller
PhilMiller previously approved these changes Aug 8, 2023
@hellkite500
Copy link
Member

Once we get all these deprecations in place, we should ensure each is represented in a single commit, then document the commit and the deprecation in a changelog.

@mattw-nws mattw-nws merged commit b57047d into NOAA-OWP:master Aug 30, 2023
19 checks passed
@jmframe
Copy link
Contributor

jmframe commented Sep 3, 2023

Looks like this removes the C++ LSTM. Have y'all been running this version? If so, were you also running the Python version? Just out of curiosity.

@mattw-nws
Copy link
Contributor Author

mattw-nws commented Sep 5, 2023

@jmframe No, we haven't in a long time [edit: the C version, specifically], that's part of the reason for the removal--I'm not even sure if it still works. The API around the formulations has been evolving and we kept having to come back to the old non-BMI models and do a lot of retrofitting--so for the time being we're removing them to streamline things, because that API is probably going to need to further evolve quite a bit for a while to deal with multiple discretizations (structured grid models, catchment models, network/routing modules, unstructured grid models...).

I think there's space for non-BMI formulations in the future for various reasons including performance optimization (relevant to LSTM, probably), but the technical debt associated with the upkeep of things that weren't directly related to the capabilities we're being asked to develop was becoming costly. I'd like this to come back in the future after the internal APIs have stabilized more.

@mattw-nws
Copy link
Contributor Author

Oh.. above was WRT the C version. The Python/BMI version is still being kept up by the formulations team and I think they're trying to build some model pieces to produce non-streamflow variables alongside it when it is used, for instance.

@jmframe
Copy link
Contributor

jmframe commented Sep 5, 2023

Sounds good. That's what I figured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants